Skip to content

Conversation

@abradley60
Copy link
Collaborator

Copy link
Collaborator

@caitlinadams caitlinadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me -- I've added some minor comments for your consideration, but the code looks functional so I'm happy to approve.

asf_user=asf_user,
asf_password=asf_password,
)
try:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but I'm wondering if eof.download.main could use a tidy up? It's a bit unintuitive that it takes both CDSE and ASF credentials at this level, although that is explained with the comment. For readability, I would find it more intuitive if it attempted to download with CDSE credentials at the time where you check if source == "CDSE" -- however, I haven't looked at the function in detail, so there may be good reasons for having the downloader handle that logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah happy to revisit later, it just means the eof.download.main function call will be written an extra time. It was a little cleaner before the try except for the .netrc had to be introduced. Think it could do with a restructure now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants